Allow handlers to return user-defined error types #1180

hawkw · 2024-11-20T22:46:29Z

Currently, all endpoint handler functions must return a
Result<T, HttpError>, where T implements HttpResponse. This is
unfortunate, as it limits how error return values are represented in
the API. There isn't presently a mechanism for an endpoint to return
a structured error value of its own which is part of the OpenAPI spec
for that endpoint. This is discussed at length in issue #39.

This branch relaxes this requirement, and instead allows endpoint
handler functions to return Result<T, E>, where E is any type that
implements a new HttpResponseError trait.

The HttpResponseError trait defines how to produce an error response
for an error value. This is implemented by dropshot's HttpError
type, but it may also be implemented by user errors. Types implementing
this trait must implement HttpResponseContent, to determine how to
generate the response body and define its schema, and they must also
implement a method HttpResponseError::status_code to provide the
status code to use for the error response. This is somewhat different
from the existing HttpCodedResponse trait, which allows successful
responses to indicate at compile time that they will always have a
particular status code, such as 201 Created. Errors are a bit different:
we would like to be able to return any number of different error status
codes, but would still like to ensure that they represent errors, in
order to correctly generate an OpenAPI document where the error schemas
are returned only for error responses (see this comment for
details). As discussed here, we ensure this by providing new
ErrorStatusCode and ClientErrorStatusCode types, which are newtypes
around http::StatusCode that only contain a 4xx or 5xx status (in the
case of ErrorStatusCode), or only contain a 4xx (in the case of
ClientErrorStatusCode). These types may be fallibly converted from an
http::StatusCode at runtime, but we also provide constants for
well-known 4xx and 5xx statuses, which can be used infallibly. The
HttpResponseError::status_code method returns an ErrorStatusCode
rather than a http::StatusCode, allowing us to ensure that error types
always have error statuses and generate a correct OpenAPI document.

Additionally, while adding ErrorStatusCodes, I've gone ahead and
changed the dropshot::HttpError type to also use it, and changed the
HttpError::for_client_error and HttpError::for_status constructors
to take a ClientErrorStatusCode. Although this is a breaking change,
it resolves a long-standing issue with these APIs: currently, they
assert that the provided status code is a 4xx internally, which is often
surprising to the user. Thus, this PR fixes #693.

Fixes #39
Fixes #693
Fixes #801

This branch is a second attempt at the change originally proposed in PR
#1164, so this closes #1164. This design is substantially simpler, and
only addresses the ability for handler functions to return
user-defined types. Other changes made in #1164, such as a way to
specify a global handler for dropshot-generated errors, and adding
headers to HttpError responses, can be addressed separately. For now,
all extractors and internal errors still produce dropshot::HttpErrors.
A subsequent change will implement a mechanism for providing alternate
presentation for such errors (such as an HTML 404 page).

Enforce that `HttpResponseError`s status codes are 4xx or 5xx using an `ErrorStatusCode` type which may only be those statuses. See: #39 (comment) While we're making breaking API changes, let's also have `HttpError::for_status` take a validated client-error-only type, rather than panicking surprisingly. This way, it's obvious to the user that the argument to this has to be a 4xx. Fixes #693

.envrc

hawkw · 2024-11-21T00:34:17Z

it occurs to me that HttpResponseHeaders might want to forward an implementation of HttpResponseError when the wrapped type implements that. I'll add one.

ahl

thanks for doing this

dropshot/examples/custom-error.rs

ahl · 2024-11-21T00:27:09Z

dropshot/src/api_description.rs

@@ -919,27 +925,59 @@ impl<Context: ServerContext> ApiDescription<Context> {
                }
            };

+            // If the endpoint defines an error type, emit that for


Alternatively, we could have added to components.responses as before and then referenced that. I can see the inline approach you've taken as potentially simpler, though it does bloat up the json output...

So, I'd like to put them in components.responses, too. The reason I didn't is that it might be a bit annoying to determine the name for each response schema. schemars internally disambiguates colliding schema names by turning subsequent ones into like Error2 or whatever, but (AFAICT) we only get that when we actually generate the schema and it gives us back a reference (into components.schemas). We could then try to parse that reference and get the name back out to then use it to generate a components.responses entry for that response, which seems possible, I just thought it seemed annoying enough that I didn't really want to bother with it. Do you think it's worth doing?

What if we name the response based on <T as JsonSchema>::schema_name()? Might that work?

Do I think it's worth doing? I think it's worth trying. It might make the code worse, but it might make the output simpler. At a minimum it will make the diffs against current json simpler. These together--I think--at least warrant giving it a shot.

I don't believe that deduplication is applied to JsonSchema::schema_name(); as
far as I can tell, it only happens once a schema has already been generated,
because that's when the generator can check if the name already exists in the
set of schemas that have been generated so far?

Ah. Unfortunately, it turns out that Progenitor chokes on outputting two separate response objects that have the same response body schema; it currently expects there to be exactly one response object for errors, as the current code hits this assertion.

I'm on the fence about whether the right solution here is to change the OpenAPI spec generation in Dropshot to emit a single response schema object for 4xx and 5xx errors, or to make Progenitor smart enough to understand that multiple response schemas can share the same response body type schema, and recognize that those are the same thing. What do you think @ahl?

What's the OpenAPI input it's choking on? I wouldn't have thought that this move away from components/responses would have had this result.

With regard to progenitor supporting it; I would like to do that, but I would also love to be able to defer that. It's not a small amount of work to do reasonably well.

Progenitor panics when building oxidecomputer/omicron#7196, which updates Omicron to depend on this branch. Note that the only change to the schemas is to generate separate response schemas for 4xx and 5xxs, as that PR hasn't added any user-defined error types or anything. It'll die on any of the OpenAPI docs in that PR, which one it hits first seems to be non-deterministic and depends on the order in which crates are built. For an example of the OpenAPI change, see https://github.com/oxidecomputer/omicron/pull/7196/files#diff-a389634108eedc06638f772e1384ef397e4d4254363b3459e5043341227f06b6.

I'm going to take another crack at having Dropshot generate one response schema for both error status codes, which would hopefully resolve the issue without requiring a change to Progenitor.

I'm genuinely surprised that progenitor is dying despite its constraints. My recollection was that a reference to a response would be handled identically to an inline response. I'm going to look on the progenitor side.

Okay @ahl, if you look at the PR comparison view again as of commit f0e6a46, you'll see that there's now no diff in any of the generated OpenAPI documents compared to main; the only diff in the expectorate output now is the new schemas with user-defined error types!

So, while it would be nice for progenitor to be able to handle cases like this, APIs that don't use custom error types will now emit backwards-compatible OpenAPI documents that the current version of Progenitor ought to be able to handle correctly.

dropshot/src/error.rs

dropshot/src/router.rs

ahl · 2024-11-21T17:42:49Z

dropshot/src/server.rs

@@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>(
                        ),


should we be firing a USDT probe here?

We weren't previously, but yeah, we probably should. I can add it in this PR if that makes sense?

Probably doesn't... (unless you've done it already). We can file an issue

dropshot/tests/fail/bad_endpoint10.stderr

dropshot/tests/fail/bad_endpoint12.stderr

dropshot_endpoint/src/endpoint.rs

Co-authored-by: Adam Leventhal <[email protected]>

ahl

nice work

dropshot/src/handler.rs

dropshot/tests/fail/bad_endpoint10.stderr

ahl · 2024-11-25T21:37:45Z

dropshot/tests/test_openapi_custom_error_types.json

+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/EnumError"


Co-authored-by: Adam Leventhal <[email protected]>

davepacheco

Very nice! Thanks again for both taking this on and sticking with it through all the design iteration. I don't have anything major here but a few things to fix up, plus: for all breaking changes I generally ask:

We should update CHANGELOG.adoc. it should have super easy-to-follow instructions there for updating across this change. There should be plenty of examples -- usually we try to say how to know you're affected and exactly what code changes to make, with an example or two.
I think it's worth doing a test-update of Omicron to a dropshot with this change. We're going to have to do it anyway and doing it first sometimes smokes out cases where the breaking change was worse than we thought to move past or stuff like that.

I did not review the bad-endpoint error output, the OpenAPI spec changes, or the dropshot_endpoint changes FWIW.

dropshot/src/error.rs

dropshot/src/test_util.rs

dropshot/tests/integration-tests/versions.rs

dropshot/tests/integration-tests/custom_errors.rs

dropshot/src/handler.rs

Co-authored-by: David Pacheco <[email protected]>

hawkw · 2024-12-03T23:58:17Z

CHANGELOG.adoc

+
+For details on how to implement `HttpResponseError` for user-defined types, see
+the trait documentation, or
+https://github.com/oxidecomputer/dropshot/blob/main/dropshot/examples/custom-error.rs[`examples/custom-error.rs`].


this link will be broken until this PR has merged --- it still felt like the nicest way to reference the example in the changelog, IMO.

hawkw · 2024-12-04T00:04:42Z

Okay, @davepacheco, I've updated Omicron to use this branch in oxidecomputer/omicron#7196, and uncovered an issue with the generated OpenAPI docs, which I've resolved (see #1180). I've added changelog entries describing the breaking changes and the migration instructions; let me know what you think of https://github.com/oxidecomputer/dropshot/blob/75cbc09542cf439412c4c13ee2b799f1dff1f28a/CHANGELOG.adoc#unreleased-changes-release-date-tbd

hawkw added 16 commits November 13, 2024 11:22

[WIP] custom error responses using HttpResponse

af226ea

use a new trait, but HttpResponseContent

4d7c3e4

hmmm maybe this is good actually

5bd7e3d

wip schema generation

2eff88a

use schemars existing deduplication

f2c7f5f

just have HttpError be a normal HttpResponseError

86b3afb

just rely on schemars to disambiguate colliding names

1f611bf

start documenting stuff

90e7247

TRYBUILD=overwrite

06a1af3

docs etc

6de6de3

remove unneeded JsonSchema impl for HttpError

cc4a2b9

theory of operation comment in error module

c513c46

actually, we can completely insulate the user from HandlerError

cfc582b

EXPECTORATE=overwrite

3d0575c

fix wsrong doctest

6b4b6d4

hawkw marked this pull request as ready for review November 21, 2024 00:04

Merge branch 'main' into eliza/custom-error-httpresponse-result

5f374b8

hawkw requested review from ahl and davepacheco November 21, 2024 00:05

hawkw added 2 commits November 20, 2024 16:11

rustfmt (NEVER use the github web merge editor)

53ed323

update to track merged changes

ab798a9

ahl reviewed Nov 21, 2024

View reviewed changes

.envrc Outdated Show resolved Hide resolved

EXPECTORATE=overwrite

e87ad82

ahl reviewed Nov 21, 2024

View reviewed changes

hawkw and others added 4 commits November 21, 2024 10:47

Apply docs suggestions from @ahl

6c9c824

Co-authored-by: Adam Leventhal <[email protected]>

remove local envrc

10a4a99

update copyright dates

b9f194c

reticulating comments

576ba5f

ahl approved these changes Nov 25, 2024

View reviewed changes

hawkw and others added 2 commits November 25, 2024 14:20

Update dropshot/src/handler.rs

a3ee555

Co-authored-by: Adam Leventhal <[email protected]>

just panic

9d99131

davepacheco reviewed Nov 27, 2024

View reviewed changes

hawkw and others added 15 commits November 27, 2024 16:17

Update error.rs

5239d17

Co-authored-by: David Pacheco <[email protected]>

Update error.rs

a3497ea

Co-authored-by: David Pacheco <[email protected]>

add test for trait-bassed custom error APIs

ab1d903

add wrong error type test with trait-based API

ea5c9ef

don't have trait-based API errors claim HttpError is required

5bf8aa2

various comment suggestions from @davepacheco

f315724

use a macro to generate ClientTestContext methods

ba5c5b3

rsutfmt

84bca39

rename HttpError::for_status to for_client_error_with_status

b017d57

document why HandlerError isnt publicly exported

72d0ced

rm spurious comma

6ad3bb4

fix typo

d189d61

move ErrorStatusCode to its own module

71dc879

improve ErrorStatusCode docs

cab73b6

rustfmt again (oops)

c86d368

hawkw added a commit to oxidecomputer/omicron that referenced this pull request Dec 3, 2024

[WIP] update to oxidecomputer/dropshot#1180

1994534

hawkw added 5 commits December 3, 2024 10:58

reuse response schemas as well as body schemas

de6933c

use schema name instead of type name when available

c6f533d

don't emit error schema for responses w/o status

6a0e990

fix accidental use of type name instead of name

f0e6a46

add changelog entries

6d6bff3

hawkw commented Dec 3, 2024

View reviewed changes

hawkw added 2 commits December 3, 2024 15:59

Merge branch 'main' into eliza/custom-error-httpresponse-result

8b648f1

oops i raced with the 0.14.0 release

75cbc09

ugh what

ba0f62a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow handlers to return user-defined error types #1180

Allow handlers to return user-defined error types #1180

hawkw commented Nov 20, 2024 •

edited

Loading

hawkw commented Nov 21, 2024

ahl left a comment

ahl Nov 21, 2024

hawkw Nov 21, 2024

ahl Nov 22, 2024

hawkw Nov 22, 2024

hawkw Dec 2, 2024

ahl Dec 3, 2024

hawkw Dec 3, 2024

ahl Dec 3, 2024

hawkw Dec 3, 2024

ahl Nov 21, 2024

hawkw Nov 21, 2024

ahl Nov 22, 2024

ahl left a comment

ahl Nov 25, 2024

davepacheco left a comment

hawkw Dec 3, 2024

hawkw commented Dec 4, 2024

		@@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>(
		),

Allow handlers to return user-defined error types #1180

Are you sure you want to change the base?

Allow handlers to return user-defined error types #1180

Conversation

hawkw commented Nov 20, 2024 • edited Loading

hawkw commented Nov 21, 2024

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Dec 4, 2024

hawkw commented Nov 20, 2024 •

edited

Loading